Strip spoofable forwarded headers at the Fastly edge entry point#505
Open
ChristianPavilonis wants to merge 1 commit intomainfrom
Open
Strip spoofable forwarded headers at the Fastly edge entry point#505ChristianPavilonis wants to merge 1 commit intomainfrom
ChristianPavilonis wants to merge 1 commit intomainfrom
Conversation
Prevent X-Forwarded-Host / Forwarded header spoofing that allows attackers to hijack URL rewriting across HTML streaming, ad-proxy URLs, Prebid OpenRTB requests, and request signing payloads. Closes #409
aram356
reviewed
Mar 16, 2026
Collaborator
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Clean, well-scoped security fix. Sanitization is placed at exactly the right point — top of route_request, before any header inspection or routing. No blockers.
Non-blocking
🏕 camp site
- Dead debug log fields (
crates/common/src/publisher.rs:229-236): After edge sanitization,x-forwarded-hostandx-forwarded-protoare alwaysNonein this debug log. These format fields now add noise without information. Suggest simplifying to:
log::debug!(
"Request info: host={}, scheme={} (Host: {:?})",
request_host,
request_scheme,
req.get_header(header::HOST),
);🌱 seedling
- Defense-in-depth for
RequestInfo::from_request:from_requeststill checks forwarded headers first in its fallback chain. The docs correctly note sanitization happens at the edge, but if a second entry point is added orfrom_requestis called without sanitizing, the old surface is exposed. A future option: accept a newtype wrapper (&SanitizedRequest) instead of&Requestto make skipping sanitization a compile error. Not needed now — just a thought for hardening later.
CI Status
- cargo fmt: PASS
- cargo test: PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- CodeQL: PASS
| // publisher-supplied Prebid scripts; the unified TSJS bundle includes Prebid.js when enabled. | ||
|
|
||
| // Extract request host and scheme from headers (supports X-Forwarded-Host/Proto for chained proxies) | ||
| // Extract request host and scheme (uses Host header and TLS detection after edge sanitization) |
Collaborator
There was a problem hiding this comment.
🏕 camp site — The debug log below (lines 229–236) still formats x-forwarded-host and x-forwarded-proto, but after edge sanitization these are always None. Consider removing those two fields to reduce noise:
log::debug!(
"Request info: host={}, scheme={} (Host: {:?})",
request_host,
request_scheme,
req.get_header(header::HOST),
);
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #409 —
X-Forwarded-Host/Forwardedheader spoofing enables URL rewrite hijack.Forwarded,X-Forwarded-Host,X-Forwarded-Proto, andFastly-SSLheaders at the top ofroute_request()before any routing or header inspection occurslog::debug!tracing when a spoofable header is actually stripped (aids incident response)RequestInfoandpublisher.rsdoc comments to reflect the new trust boundaryWhy it's safe to remove these headers
On Fastly Compute, the service is the edge — there is no legitimate upstream proxy setting forwarded headers. Any
X-Forwarded-HostorForwardedvalues present in the request were sent directly by the client and are therefore untrusted.After stripping, the existing fallback logic in
RequestInfo::from_requestnaturally lands on trustworthy signals:extract_request_hostfalls throughForwarded(gone) →X-Forwarded-Host(gone) →Hostheader — which Fastly validates against the service configurationdetect_request_schemehits Fastly SDK TLS detection first (priority 1, unchanged by this PR, not a header — it's SDK-level and unspoofable) → forwarded headers (gone) → defaulthttpThe forwarded header parsing code in
commonremains intact for potential non-edge deployment contexts, but the Fastly entry point now enforces the trust boundary.Impact on integrations
site.domain/site.pagesettings.publisher.domain, notRequestInfoHostheader is the actual browser-facing domainext.trusted_serverboth use the same sourceRequestInfoat allHostis the correct domainGit blame context
The forwarded header logic was introduced in commit
563c281("Improve forwarded header parsing for host and scheme") with no security review (PR #176 had an empty description and no review comments). It was speculative support for a hypothetical chained-proxy scenario (Client → Proxy A → Trusted Server) that does not apply to the Fastly Compute deployment model.Test plan
sanitize_removes_all_spoofable_headers,sanitize_then_request_info_falls_back_to_hostRequestInfoforwarded-header parsing tests, which validate thecommoncrate logic independently)cargo fmt,cargo clippyclean